Implement container that gives additional information on the profile setting by parttimenerd · Pull Request #4222 · firefox-devtools/profiler

parttimenerd · 2022-09-07T10:03:31Z

This implements a container giving more information than already provided in the "Profile Info" container (see #4209). This gives us the possibility to add more information that is related to the whole profile without cluttering the "Profile Info" container.

This adds a new "Additional Information" button:

When you click on it, you see a floating container with additional information:

The information is specified in the new extra field in the meta section of the profile:

export type ExtraProfileInfoSection = {|
  // section label
  label: string,
  entries: {|
    label: string,
    format: MarkerFormatType,
    // any value valid for the formatter
    value: any,
  |}[],
|};
...
  extra?: ExtraProfileInfoSection[],
...

Sample profile

Deployment Preview

codecov · 2022-09-07T10:16:02Z

Codecov Report

Base: 88.44% // Head: 88.48% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (feb442b) compared to base (14d46cc).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head feb442b differs from pull request most recent head b0211d0. Consider uploading reports for the commit b0211d0 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4222      +/-   ##
==========================================
+ Coverage   88.44%   88.48%   +0.03%     
==========================================
  Files         282      282              
  Lines       24858    24850       -8     
  Branches     6660     6656       -4     
==========================================
+ Hits        21986    21988       +2     
+ Misses       2667     2660       -7     
+ Partials      205      202       -3

Impacted Files	Coverage Δ
src/components/app/MenuButtons/MetaInfo.js	`85.41% <100.00%> (+2.70%)`	⬆️
src/selectors/profile.js	`96.21% <100.00%> (+0.01%)`	⬆️
src/profile-logic/symbol-store.js	`92.98% <0.00%> (-0.24%)`	⬇️
src/components/shared/SourceView-codemirror.js	`90.81% <0.00%> (-0.10%)`	⬇️
src/profile-logic/symbolication.js	`86.91% <0.00%> (-0.09%)`	⬇️
src/profile-logic/processed-profile-versioning.js	`85.21% <0.00%> (-0.07%)`	⬇️
src/profile-logic/merge-compare.js	`92.74% <0.00%> (-0.03%)`	⬇️
src/test/fixtures/profiles/processed-profile.js	`94.48% <0.00%> (-0.01%)`	⬇️
src/profile-logic/data-structures.js	`95.45% <0.00%> (ø)`
src/profile-logic/mozilla-symbolication-api.js	`13.09% <0.00%> (+1.39%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

canova

For the profile data format:
I still have doubts about using MarkerFormatType on other places but maybe in the future we can make this more generic across profile. So I'm not opposed to it.

For the UI:
Is there a need for a new overlaying popup exactly? I'm surprised by this because I was always thinking that we can use the same popup and never though of another poup. Adding another popup brings accesibility issues like, how to close, how to go back to previous popup and confusion of opening another overlay from an overlay etc.

I was thinking about expanding the same "profile info" popup with additional information when user clicks on this button. There are <details> and <summary> elements you can use for this as well. This also removes a lot of extra css and code from the PR.
What do you think?

parttimenerd · 2022-09-08T12:57:48Z

I'm surprised by this because I was always thinking that we can use the same popup and never though of another poup.

Which popup? The current "Profile info" popup is pretty narrow and small. The popup can be opened and closed the same way as the "Profile info" popup.

I would like to present lots of information in this popup. Like all currently running processes on the host system, all environment variables (might have fairly long names), all used libraries.

The CSS (and code) is mostly copied from the existing "Profile info" popup.

canova · 2022-09-08T13:21:20Z

Do you have an example where this width is not enough for your use case? Since you are moving the value to a new line, you will have more space horizontally. In your current example, it looks like they would all fit.

The problem with the new popup is that it's not obvious that which element it is tied to. For the profile info popup it's obvious that it's tied to "Profile Info" button:

As you can see, there is a clear arrow and the button is in selected state.

But opening a popup from another popup doesn't seem right UX wise.
Also your new section seems more like a modal (especially considering that it dims the outside area). A modal should have a proper close buttons and properly handle the keyboard/focus state for accessibility.
We have a modal currently for keyboard shortcuts, you can it if you press ?:

In this one there is a clear close button, clear title, and the focus state is handled properly.

I also don't think we should use the same modal for this case though.
Lastly I don't think we should add another panel that 99% of our users won't see. This will make future development prone to errors and we'll have to maintain it in the future. I feel like the simplest solution of expanding the current panel is a better compromise for both of us.

parttimenerd · 2022-09-08T13:28:07Z

Lastly I don't think we should add another panel that 99% of our users won't see. This will make future development prone to errors and we'll have to maintain it in the future. I feel like the simplest solution of expanding the current panel is a better compromise for both of us.

You're probably right.

I currently don't have a case at hand where expanding the current popup to the bottom, and adding scrollbars would not help.

Another idea: Could we extend the "Profile info" to the right? This would alleviate all my width concerns.

canova · 2022-09-08T16:20:29Z

Another idea: Could we extend the "Profile info" to the right? This would alleviate all my width concerns.

I'm not opposed to it as long as it doesn't make our other side by side key-value items hard to read. Could be good to get feedback from others too.

parttimenerd · 2022-09-12T11:13:30Z

julienw

Thanks for the patch, and for your patience!
I left a few comments, tell me what you think.

First, an optional quick note: we could use a <details> element, which supports this behavior natively and would make it possible to remove the showsMoreInfo state. I'm not sure about the stylability, but it would be good to try using it.

About the formatting abilities, ideally I'd prefer that we extract the formatting mechanisms to another file before we move forward with this PR (and probably also the other one). But if that's too much work it's also good to me that we move forward with this solution and we clean up later. Please tell me your thoughts!

Also it would be good to have a test that exercize this new code. There's a block about the MetaInfo panel in src/test/components/MenuButtons.test.js that you can insert a test to.

Please squash your 2 existing commits and add your review fixes to an extra commit for an easier second review :-)

parttimenerd · 2022-09-26T11:58:33Z

First, an optional quick note: we could use a
Details
element, which supports this behavior natively and would make it possible to remove the showsMoreInfo state. I'm not sure about the stylability, but it would be good to try using it.

I want to still change the name of the button. So I'll still want to use the state. I could achieve this by setting CSS attributes to the localized strings, but I think this would be hard to understand.

About the formatting abilities, ideally I'd prefer that we extract the formatting mechanisms to another file before we move forward with this PR (and probably also the other one). But if that's too much work it's also good to me that we move forward with this solution and we clean up later. Please tell me your thoughts!

I'd rather like to have it in a later PR (submitted directly after merging this PR and #4201), as it would require many rebases and refactoring is not really the purpose of this PR.

julienw · 2022-09-26T14:10:11Z

I'd rather like to have it in a later PR (submitted directly after merging this PR and #4201), as it would require many rebases and refactoring is not really the purpose of this PR.

You shouldn't be afraid of rebases :-)

Do the refactoring in another PR.
Rebase this patch on top of the other PR.
Change import and calls to formatters.

My understanding is that this would be about the same amount of work.

That said I'm OK with your approach.

julienw

I pushed some changes, can you please have a quick look ? :) thanks

parttimenerd · 2022-09-29T18:54:16Z

Your changes look good to me.

parttimenerd · 2022-09-29T19:21:09Z

Short objection: Why does it show "Hide" and not "Show Less"? I find the latter better.

julienw

looks good with my changes (that I discussed with Johannes as well)

outdated

parttimenerd requested a review from a team as a code owner September 7, 2022 10:03

parttimenerd force-pushed the parttimenerd_more_info_panel branch 2 times, most recently from fa4c413 to 24eb3f5 Compare September 7, 2022 10:09

canova previously requested changes Sep 8, 2022

View reviewed changes

Comment thread src/types/profile.js Outdated

flodolo reviewed Sep 8, 2022

View reviewed changes

Comment thread locales/en-US/app.ftl Outdated

parttimenerd force-pushed the parttimenerd_more_info_panel branch from 892759b to da3298e Compare September 15, 2022 15:05

parttimenerd requested review from canova and flodolo and removed request for canova and flodolo September 21, 2022 08:27

julienw requested changes Sep 23, 2022

View reviewed changes

Squashed prior commits

9f749b6

Improve and add tests

983e8af

parttimenerd force-pushed the parttimenerd_more_info_panel branch from da3298e to 983e8af Compare September 26, 2022 13:21

parttimenerd mentioned this pull request Sep 28, 2022

Make formatting types generic and extensible #4250

Open

julienw self-requested a review September 28, 2022 09:23

Improve tests

8460910

julienw reviewed Sep 29, 2022

View reviewed changes

Comment thread src/components/app/MenuButtons/MetaInfo.js Outdated

julienw reviewed Sep 29, 2022

View reviewed changes

Comment thread src/test/components/MenuButtons.test.js

julienw reviewed Sep 29, 2022

View reviewed changes

Comment thread src/components/app/MenuButtons/MetaInfo.css Outdated

julienw reviewed Sep 29, 2022

View reviewed changes

Comment thread src/components/app/MenuButtons/MetaInfo.js

flodolo approved these changes Sep 29, 2022

View reviewed changes

julienw added 2 commits September 30, 2022 12:08

Simplify the implementation of the more info button

6ee1566

Active hover and active styles for the details fake button

3cc8b5f

julienw force-pushed the parttimenerd_more_info_panel branch from 279cdb5 to 3cc8b5f Compare September 30, 2022 10:09

julienw approved these changes Sep 30, 2022

View reviewed changes

Merge branch 'main' into parttimenerd_more_info_panel

feb442b

julienw enabled auto-merge September 30, 2022 10:30

Merge branch 'main' into parttimenerd_more_info_panel

b0211d0

julienw merged commit 244f5be into firefox-devtools:main Oct 11, 2022

Conversation

parttimenerd commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

canova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

parttimenerd commented Sep 8, 2022

Uh oh!

Uh oh!

canova commented Sep 8, 2022

Uh oh!

parttimenerd commented Sep 8, 2022

Uh oh!

canova commented Sep 8, 2022

Uh oh!

parttimenerd commented Sep 12, 2022

Uh oh!

julienw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

parttimenerd commented Sep 26, 2022

Uh oh!

julienw commented Sep 26, 2022

Uh oh!

julienw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

parttimenerd commented Sep 29, 2022

Uh oh!

parttimenerd commented Sep 29, 2022

Uh oh!

julienw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

parttimenerd commented Sep 7, 2022 •

edited

Loading

codecov Bot commented Sep 7, 2022 •

edited

Loading